Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QGDS-22-Accordion Init #247

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

QGDS-22-Accordion Init #247

wants to merge 2 commits into from

Conversation

SenChung
Copy link

Init: Accordion file restructured.
note: Will need to look into the global.js file, as there are a few strange things happening with multiple accordions on the page.

@SenChung SenChung added the documentation Improvements or additions to documentation label Jan 21, 2025
@SenChung SenChung requested a review from stvp-qld January 21, 2025 06:27
@SenChung SenChung self-assigned this Jan 21, 2025
Copy link
Member

@duttonw duttonw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hbs cleanup is interesting but we do need to think about how QH and many others in handling the migration to the new template. i.e. migration guide required.

Also how to include this in a mega json array that has a simple template that auto loads any objects dynamically. see https://66c421d9ea5f69287ba33241-zarbnywjry.chromatic.com/?path=/story/1-styles-typography--component-global-elements which is run off https://github.com/qld-gov-au/qgds-vanilla/blob/develop/src/stories/templates/global-body/global-body-all_typography.json

{{{intro}}}
{{/if}}

<div class="qld__accordion-group qld__accordion-group--{{theme}}" id="accordion-group--{{id}}">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to move away from qld__accordion-group--{{theme}} to just using the 'qld-{{theme}}' to css bloat.

</div>
{{/if}}
<ul class="qld__accordion-list">
{{#each component}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you have dropped {{#printAccordion metadata}}
did you work out what that was?

import mockupData from "./accordion.data.json";

export default {
title: "Components / Accordion",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was title: "Components/Accordion",

title: "Components / Accordion",
render: (args) => {
try {
return new QGDS.Accordion({ data: args }).htmlstring;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is over engineered, just do

render: ( args) => {
        handlebarsInit(Handlebars)
        try {
            return Handlebars.compile("{{> accordion }}")(args)
        } catch (e) {
            console.log(e)
            return JSON.stringify(e) + JSON.stringify(args);
        }
    },

options: ["light", "alt", "dark", "dark-alt"],
},
},
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please put back the figma links

 /**
     * Additional parameters for the story.
     *
     * @type {Object}
     * @property {Object} design - Configuration for the design parameter.
     * @property {string} design.name - Name of the design parameter.
     * @property {string} design.type - Type of the design parameter.
     * @property {string} design.url - URL of the design parameter.
     */
    parameters: {
        design: {
            name: "QGDS Figma Reference",
            type: "figma",
            url: "https://www.figma.com/design/qKsxl3ogIlBp7dafgxXuCA/QLD-GOV-DDS?node-id=23167-395554",
        },
    },

theme: "dark-alt",
id: "131",
},
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add back in the example html versions for css diffing.


/**
 * Default Accordion story
 */
export const Default = {};

/**
 * Single light
 */
export const SingleLight = {
    render: () => {
        return example2html;

    },
    args: {},
    globals: { theme: 'None'},

}
/**
 * Single dark
 */
export const SingleDark = {
    render: () => {
        return example3html;

    },
    args: {},
    globals: { theme: 'Dark'},

}
/**
 * Multi light
 */
export const MultiLight = {
    render: () => {
        return example4html;

    },
    args: {},
    globals: { theme: 'None'},

}
/**
 * Single dark
 */
export const MultiDark = {
    render: () => {
        return example5html;

    },
    args: {},
    globals: { theme: 'Dark' },

}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thought this would have broken https://github.com/qld-gov-au/qgds-vanilla/blob/develop/src/components/_global/css/body/stories/allTypography.json but its not used there.

Be aware that the move to removing the .data. wrapper does make it harder to do have a global template and just pass in a huge json file. like
https://github.com/qld-gov-au/qgds-vanilla/blob/develop/src/stories/templates/global-body/global-body-all_typography.json
e.g.

{
  "items": [
    {
      "component": {
        "type": "h1",
        "data": {
          "metadata": {
            "id_field": {
              "value": "h1id"
            }
          },
          "content": {
            "value": "Heading 1"
          }
        }
      }
    },

now cant be auto including this template as its not conformant to the pattern used everywhere else.

unsure how we should go forward on this. i.e. have this called internal which is more streamlined and have the accordion hbs re jig the array to pass through, or a js helper function added to do the rejig pass through to stay backwards compatible.

@@ -0,0 +1,8 @@
/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this for building or for browser runtime?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please not delete, this is developer docs on accordion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please not delete and look at streamlining to just use qld--alt, qld--dark, qld--alt, instead of the double wrap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants